Skip to content

Do not add -sysroot flag when ANDROID_NDK_ROOT environment is set #1879

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Do not add -sysroot flag when ANDROID_NDK_ROOT environment is set #1879

wants to merge 1 commit into from
+0 −78

Conversation

marcprux
Copy link

When the environment ANDROID_NDK_ROOT is set, the driver manually adds it as a --sysroot flag. This breaks building with an Android SDK when this variable is set, with confusing errors. I first saw this at finagolfin/swift-android-sdk#207 (comment), and more recently ran into it again (https://github.com/swift-android-sdk/swift-docker/actions/runs/14584558227/job/40907674425) while working on the official Android SDK build script, where it fails a build with:

<unknown>:0: error: missing required module 'SwiftAndroid'

In both cases, manually un-setting ANDROID_NDK_ROOT (which is set by default in GitHub workflows) resolved the issue.

This flag has some history (#1560, #1681, and #1811), but I feel like it should just be excised altogether since the Android SDK can specify its own sdkRootPath in the swift-sdk.json file. If Windows needs it due to lack of Swift SDK support, then it could be gated inside an #if os(Windows) check.

And BTW, the #if arch(x86_64) check is incorrect: the Android NDK works on both Intel and ARM macOS machines, despite being stored under "darwin-x86_64", as the binaries are universal:

zap ~ % file $ANDROID_HOME/ndk/27.0.12077973/toolchains/llvm/prebuilt/darwin-x86_64/bin/clang
~/Library/Android/sdk/ndk/27.0.12077973/toolchains/llvm/prebuilt/darwin-x86_64/bin/clang: Mach-O universal binary with 2 architectures: [x86_64:Mach-O 64-bit executable x86_64] [arm64]
~/Library/Android/sdk/ndk/27.0.12077973/toolchains/llvm/prebuilt/darwin-x86_64/bin/clang (for architecture x86_64):	Mach-O 64-bit executable x86_64
~/Library/Android/sdk/ndk/27.0.12077973/toolchains/llvm/prebuilt/darwin-x86_64/bin/clang (for architecture arm64):	Mach-O 64-bit executable arm64

@marcprux marcprux mentioned this pull request Apr 22, 2025
Open
5 tasks
marcprux added a commit to skiptools/swift-android-native that referenced this pull request May 13, 2025
@marcprux
Unset ANDROID_NDK_ROOT for CI (swiftlang/swift-driver#1879)
2556db0
@marcprux marcprux mentioned this pull request May 13, 2025
Open
@marcprux marcprux marked this pull request as ready for review May 14, 2025 12:21
@marcprux
Copy link
Author

I've marked this ready for review, @jakepetroules, @finagolfin, and @compnerd.

See also the discussion at swiftlang/swift-build#495 (comment)

@finagolfin
Copy link
Member

I agree with getting rid of this. As much as the idea is to automatically set things up and make things automatically work, I think it's better to have the user configure this themselves. For example, this env variable is automatically set on the github CI, so even if the user explicitly configures the sdkRootPath to some other NDK, that will be overridden by this environment variable.

Better to not have such magic and make the setup more explicit, as it would be after this pull.

@compnerd
Copy link
Member

This is going to break all of our builds, please do not commit this as is. We need the ability to be able to pass the -sysroot through. I don't think that this is the right approach.

@marcprux
Copy link
Author

This is going to break all of our builds

Then how about gating it within an #if os(Windows) check? This issue a blocker for Android builds on macOS and Linux, so we need some solution.

@jakepetroules
Copy link
Contributor

This is going to break all of our builds, please do not commit this as is. We need the ability to be able to pass the -sysroot through. I don't think that this is the right approach.

This change isn't removing the ability to do that, right? It's just removing the default fallback. From what I understand, explicitly passing -sysroot is still gonna work, and that's what we want build systems to do anyways. Am I understanding incorrectly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@marcprux @finagolfin @compnerd @jakepetroules